chore: numeric audit 0#20491
Conversation
| } | ||
| auto lo = static_cast<uint64_t>(u); | ||
| return static_cast<size_t>(__builtin_clzll(lo)) + 64; | ||
| if (lo != 0U) { |
There was a problem hiding this comment.
its technically undefined behavior to call __builtin_clzll on 0 so adding this extra protection (matches what we do for u256 below)
| if (lower_bound == in) { | ||
| return in; | ||
| } | ||
| // Overflow check: lower_bound is the highest power of 2 <= in, |
There was a problem hiding this comment.
probably overly cautious but avoids wrap in lower_bound * 2 if input is > numeric_limits/2
| constexpr uint128_t& operator=(uint128_t&& other) = default; | ||
| constexpr ~uint128_t() = default; | ||
| explicit constexpr operator bool() const { return static_cast<bool>(data[0]); }; | ||
| explicit constexpr operator bool() const |
There was a problem hiding this comment.
bug: was only checking lowest limb. wasn't ever showing up anywhere because we don't have instances where we directly treat a u128 as a bool
There was a problem hiding this comment.
maybe worth documenting that it converts any non-zero value to 1?
| // NOTE: this instantiation is only used to maintain a 1024 barrett reduction test. | ||
| // The simpler route would have been to delete that test as this modulus is otherwise not used, but this was more | ||
| // conservative. | ||
| constexpr uint512_t TEST_MODULUS(uint256_t{ "0x04689e957a1242c84a50189c6d96cadca602072d09eac1013b5458a2275d69b1" }, |
There was a problem hiding this comment.
moved to test suite
|
|
||
| ~uintx() = default; | ||
| constexpr explicit operator bool() const { return static_cast<bool>(lo); }; | ||
| constexpr explicit operator bool() const { return static_cast<bool>(lo) || static_cast<bool>(hi); }; |
There was a problem hiding this comment.
same bug as in u128
| constexpr uintx slice(const uint64_t start, const uint64_t end) const | ||
| { | ||
| const uint64_t range = end - start; | ||
| const uintx mask = range == base_uint::length() ? -uintx(1) : (uintx(1) << range) - 1; |
There was a problem hiding this comment.
bug: in the first branch of this conditional (range == base_uint::length()) the mask is twice as large as it should be. I.e. if range == base_uint::length() == 256, the mask is u512(-1) when it should be u256(-1). The second branch in the conditional behaves correctly for this case so I'm simply removing the conditional
| explicit constexpr operator bool() const { return static_cast<bool>(data[0]); }; | ||
| explicit constexpr operator bool() const | ||
| { | ||
| return (data[0] != 0) || (data[1] != 0) || (data[2] != 0) || (data[3] != 0); |
iakovenkos
left a comment
iakovenkos
left a comment
There was a problem hiding this comment.
lgtm! the only question I have is re the bool cast. in circuit we def don't want to cast any non-zero value to 1, but maybe it's a standard convention for native arithmetic
Yeah AFAIK "true iff !=0" is the universal behavior for integer types being cast to a bool so I think anything else would be confusing. Its marked explicit so it can only happen in truly boolean contexts so I think we're good |
BEGIN_COMMIT_OVERRIDE chore: chonk rec ver 0 (#20506) fix: allocate non-gate selectors at trace-active size instead of dyadic size (#20600) chore: numeric audit 0 (#20491) chore: prepare barretenberg-rs for crates.io publishing (#20496) chore: add build_bench to ci-barretenberg-full (#20650) chore: add component graphs for app-proving benchmarks (#20649) END_COMMIT_OVERRIDE
2 participants
Audit and cleanup up of
barretenberg/numericmodule. Largely adding some protections, tests and docs clarifications. Also some minor bug fixes:bool()inuint128,uint256,uintxonly checked the lowest limb/half; now checks all limbsuintx::slice()(full width instead of half-width whenrange == base_uint::length())